Support variables in #:project directives
#51108
Conversation
jjonescz
force-pushed
the
sprint-project-vars
branch
from
9ad9ae2 to
bc17aea
Compare
There was a problem hiding this comment.
Pull Request Overview
Adds support for MSBuild variable expansion inside #:project directives and updates tests and documentation accordingly.
- Introduces directive evaluation pass (EvaluateDirectives) to expand MSBuild variables and resolve project paths.
- Extends CSharpDirective.Project to preserve original and unresolved names for later path adjustments during project conversion.
- Adds tests covering variable usage and updates documentation about variable handling limitations.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| RunTelemetryTests.cs | Adapts tests to new CSharpDirective.Project constructor. |
| RunFileTests.cs | Adds tests for variable-based project references and malformed variable syntax. |
| DotnetProjectConvertTests.cs | Adds test cases for variable-containing paths; updates expectations for cross-platform separators. |
| VirtualProjectBuildingCommand.cs | Adds directive evaluation, caching of source file, and enhanced project directive handling. |
| ProjectConvertCommand.cs | Integrates directive evaluation and updates path rewrite logic to preserve variable intent. |
| dotnet-run-file.md | Documents variable support and caveats for #:project directives. |
Co-authored-by: Copilot <[email protected]>
|
The linked issue has milestone 10.0.2xx, was this PR meant to target release/10.0.2xx branch? |
|
Definitely, I was worried there might be changes missing in the 2xx branch, but looks like it's fairly up to date wrt file-based app PRs, so I can retarget now. |
This comment was marked as outdated.
This comment was marked as outdated.
|
@RikkiGibson @333fred @MiYanni for reviews, thanks |
src/Cli/dotnet/Commands/Project/Convert/ProjectConvertCommand.cs
Outdated
Show resolved
Hide resolved
| ? (project is null ? p : p.WithName(project.ExpandString(p.Name))) | ||
| .ResolveProjectPath(sourceFile, diagnostics) |
There was a problem hiding this comment.
Nit: The original formatting here makes what ResolveProjectPath was being invoked on hard to see. Consider something more like this.
| ? (project is null ? p : p.WithName(project.ExpandString(p.Name))) | |
| .ResolveProjectPath(sourceFile, diagnostics) | |
| ? (project is null | |
| ? p | |
| : p.WithName(project.ExpandString(p.Name))) | |
| .ResolveProjectPath(sourceFile, diagnostics) |
|
|
||
| public Project WithName(string name, bool preserveUnresolvedName = false) | ||
| { | ||
| return name == Name |
There was a problem hiding this comment.
It feels like we would also want to create a new instance if !preserveUnresolvedName && UnresolvedName != name. If we don't expect this to occur, it would probably be good to add an assertion to that effect.
There was a problem hiding this comment.
Good catch, forgot to update this when adding the parameter.
|
|
||
| // If the original path is to a directory, just append the resolved file name | ||
| // but preserve the variables from the original, e.g., `../$(..)/Directory/Project.csproj`. | ||
| if (Directory.Exists(project.UnresolvedName)) |
There was a problem hiding this comment.
If I am understanding right, project.UnresolvedName could be relative, creating a dependency on the process current directory. I think we should avoid that, e.g. by explicitly resolving relative to some known absolute path, like the Path of the containing SourceFile, before doing this check.
There was a problem hiding this comment.
Good catch, thanks, I will add a test for this too.
4 participants
Closes #49286.